Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Phase 3.D PR 4-B-3a — htfifo capability poller. Pure-additive helper that PR 5's CreateQueue gate will consume; htfifoCapabilityAdvertised stays false (PR 4-B-3b will flip it after the leadership-refusal hook lands). PR description has the full self-review and test plan.

Claude finished @bootjp's task in 2m 18s —— View job

Code Review: feat(sqs): htfifo capability poller (Phase 3.D PR 4-B-3a)

Tasks:

• Read changed files
• Review implementation (adapter/sqs_capability_poller.go)
• Review tests (adapter/sqs_capability_poller_test.go)
• Check consistency with existing SQS adapter patterns
• Post final review

Verdict: Approve with minor notes. The implementation is correct. The concurrency model is clean, the fail-closed contract is consistent, and the test coverage is thorough. A few small issues worth calling out below, none blocking.

Implementation (adapter/sqs_capability_poller.go)

Goroutine + channel model (lines 114–137) — Correct. Buffered channel sized to len(peers) so every goroutine can send without blocking; wg.Wait() before close(results) guarantees the range-drain sees all results. Race-free slice writes via the indexed channel. No goroutine leaks in any path (parent-ctx cancellation or per-peer timeout both return through pollOneSQSPeerForHTFIFO, which sends its result before returning).

Per-peer timeout (line 153) — context.WithTimeout(ctx, defaultSQSCapabilityPollTimeout) takes the shorter of the caller's deadline and now+3s, which is the right semantics.

Body cap (line 176) — Consistent with how s3.go:1523 does it (io.LimitReader); sqs_auth.go uses http.MaxBytesReader instead but the effect is the same in this direction (outbound client read vs. inbound server read). Fine as-is.

Error string on fmt.Sprintf vs. errors.Wrapf (lines 172–173, 184–185) — The status.Error field is a string, not a error, so wrapcheck doesn't apply here. The mixed usage (errors.Wrapf(...).Error() for transport errors, fmt.Sprintf for logic errors) is fine.

Minor issue 1 — Non-200 body not drained

// adapter/sqs_capability_poller.go:171-174
if resp.StatusCode != http.StatusOK {
    status.Error = fmt.Sprintf(...)
    return status  // body is closed but not drained
}

resp.Body.Close() without draining prevents the HTTP transport from reusing the TCP connection. In a control-plane path (one call per CreateQueue) this is acceptable, but if the gate ever gets called on a cluster of many nodes under load, this will force connection teardown on every non-200 peer response. Adding _, _ = io.Copy(io.Discard, resp.Body) before the early return is the idiomatic fix. Fix this →

Minor issue 2 — buildSQSHealthURL double-path edge case

// adapter/sqs_capability_poller.go:209-214
func buildSQSHealthURL(peer string) string {
    if strings.HasPrefix(peer, "http://") || strings.HasPrefix(peer, "https://") {
        return strings.TrimRight(peer, "/") + sqsHealthPath
    }
    return "http://" + peer + sqsHealthPath
}

A caller who passes "http://node:5050/sqs_health" (full URL already including the path) would receive "http://node:5050/sqs_health/sqs_health" — the path is doubled. The contract ("pass a base URL") is documented in the comment and the test cases, so this isn't a latent bug in any callers today. But since TestBuildSQSHealthURL is specifically the regression fixture for this function's edge cases, it's worth adding a "http://node:5050/sqs_health" case that documents the behavior (whether you decide to detect+reject it or just document that callers must not do it). Fix this →

Tests (adapter/sqs_capability_poller_test.go)

Coverage is excellent — every documented failure mode is pinned, concurrent polling is timed, URL construction has its own table, body-size cap is independently exercised.

Nit — TestPollSQSHTFIFOCapability_TimeoutFailsClosed tests parent-context cancellation, not the per-peer cap

// adapter/sqs_capability_poller_test.go:147-151
ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
defer cancel()
report := PollSQSHTFIFOCapability(ctx, nil, []string{addrHang})

The comment says "force the per-peer timeout path quickly", but what this test actually exercises is ctx expiring at 500ms (which is shorter than defaultSQSCapabilityPollTimeout = 3s). The context.WithTimeout(ctx, 3s) inside pollOneSQSPeerForHTFIFO inherits the shorter deadline from the parent, so the request times out via the parent context, not the per-peer cap. The behavior pinned is correct (fail-closed), but the per-peer defaultSQSCapabilityPollTimeout = 3s path is never independently exercised. A complementary test that passes context.Background() (no deadline) and a peer that hangs for 3.5s would close this gap and validate the per-peer cap independently. Not a bug — just a gap in the stated coverage.

Nit — TestPollSQSHTFIFOCapability_OneMissingFailsClosed peer ordering assumption

// adapter/sqs_capability_poller_test.go:73-80
require.True(t, report.Peers[0].HasHTFIFO)   // addrGood
require.False(t, report.Peers[1].HasHTFIFO)  // addrOld

The indexed-channel design (goroutine writes to results at its input index) makes peer-order deterministic in the output slice, so these assertions are correctly testing the indexed aggregation. Calling this out because it's a subtle property worth keeping — if pollOneSQSPeerForHTFIFO is ever inlined into the goroutine using a shared-map instead of the indexed channel, this test would start flapping. The test is a good sentinel for that regression.

Summary

@claude review

Round 2 — addresses Round 1 review:

1. Gemini medium — Body.Close error silently ignored: now logs via slog.Warn with peer address. A failed Close means the http.Transport will tear down rather than reuse the connection — silently dropping it could mask leaking connections under load.

2. Claude minor — non-200 body not drained: io.Copy(io.Discard, ...) before the early return so the transport can reuse the connection. Drain is wrapped in the same io.LimitReader cap so a misbehaving peer can't drain memory through the discard path.

3. Claude nit — per-peer cap not exercised independently: refactored PollSQSHTFIFOCapability signature to (ctx, peers, cfg PollerConfig) where PollerConfig.PerPeerTimeout is a tunable. Tests now have:

• TestPollSQSHTFIFOCapability_ParentContextDeadlineFailsClosed (renamed from TimeoutFailsClosed) — pins parent-ctx deadline path
• TestPollSQSHTFIFOCapability_PerPeerTimeoutFailsClosed (new) — uses context.Background() + PerPeerTimeout=100ms to exercise the cap independently of any parent deadline

Bonus: PerPeerTimeout is also a useful operator knob — different cluster latencies want different bounds. Audit per lessons-learned: grep -rn PollSQSHTFIFOCapability confirmed only the test file references the function — API change is safe.

4. Claude minor — buildSQSHealthURL double-path edge case: added an explicit test case documenting the behavior (caller must not pass a URL that already includes /sqs_health).

go test -race ./adapter/... and golangci-lint clean.

Claude finished @bootjp's task in 2m 8s —— View job

Code Review: Round 2 — feat(sqs): htfifo capability poller

Tasks:

• Read changed files
• Review Round 2 changes against Round 1 feedback
• Check implementation correctness
• Post final review

Verdict: LGTM. All four Round 1 items are correctly addressed. No new issues found.

Round 1 feedback checklist

1. Body.Close() error logged (Gemini medium) ✅

// adapter/sqs_capability_poller.go:204-209
defer func() {
    if cerr := resp.Body.Close(); cerr != nil {
        slog.Warn("sqs capability poller: response body close failed",
            "peer", peer, "err", cerr)
    }
}()

Correct. Uses "peer" and "err" keys consistent with sqs_catalog.go and redis_retry.go. The closure captures peer from the enclosing scope correctly. The defer fires after every return path, so all cleanup is centralized.

2. Non-200 body drained before early return (Claude minor) ✅

// adapter/sqs_capability_poller.go:211-215
if resp.StatusCode != http.StatusOK {
    _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, sqsCapabilityMaxBodyBytes))
    status.Error = fmt.Sprintf("%s returned HTTP %d", url, resp.StatusCode)
    return status
}

Correct. The drain is bounded by the same 1 KiB LimitReader as the success path — a misbehaving peer can't exhaust memory through the discard. After the drain, the deferred Close() fires without needing to consume the remainder itself (on a normally-behaved peer, the body is already empty; on an oversized one, the transport tears down the connection). Idiomatic _, _ = for discard.

3. PollerConfig.PerPeerTimeout; per-peer cap exercised independently (Claude nit) ✅

The PollerConfig struct is clean — zero value is safe, all fields optional. The renamed TestPollSQSHTFIFOCapability_ParentContextDeadlineFailsClosed and new TestPollSQSHTFIFOCapability_PerPeerTimeoutFailsClosed now test two distinct code paths:

// Parent-ctx path (line 143): ctx expires at 500ms < 3s per-peer cap
ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)

// Per-peer-cap path (line 179-180): context.Background() + 100ms cap
report := PollSQSHTFIFOCapability(context.Background(), []string{addrHang},
    PollerConfig{PerPeerTimeout: 100 * time.Millisecond})

The < 2*time.Second bound for the 100ms cap test is generous enough to avoid CI flakiness but tight enough to catch a regression to unbounded behavior. The boundary condition perPeerTimeout <= 0 → defaultSQSCapabilityPollTimeout correctly handles the zero value.

4. buildSQSHealthURL double-path documented in test (Claude minor) ✅

// adapter/sqs_capability_poller_test.go:291-297
{
    "http://node.example:5050" + sqsHealthPath,
    "http://node.example:5050" + sqsHealthPath + sqsHealthPath,
},

The comment ("pin the behaviour so a future refactor can either keep it or change the contract intentionally") is the right framing — document and lock rather than silently accept. No caller today passes a full request URL to the poller, so this is a latent trap, not an active bug.

One micro-observation (not blocking)

The slog error key is "err" here (consistent with sqs_catalog.go, redis_retry.go) but redis_delta_compactor.go uses "error". There's a pre-existing inconsistency in the adapter package; this PR is consistent with the SQS-adjacent files, so no change needed here.

Summary

The concurrency model, fail-closed contract, body-size cap, and all test coverage from Round 1 remain intact. Ready to merge.